Clean, minimal, well-designed. Plumbs an existing producer field through the CLI rather than inventing new mechanism.

What I checked

1. The producer-side claim is real. packages/producer/src/server.ts:77 already declares entryFile?: string on RenderJobOptions, parses it at line 100-103 (trim + truthy check), validates existence at line 124-126, and falls back to "index.html" at line 122. The PR's claim of "leverages the existing entryFile config — no engine changes needed" matches the source. ✓

2. Backward compat. args.composition?.trim() || undefined at line 273 — empty string and whitespace-only treated as not-set. Producer defaults to index.html when undefined. Existing renders without the flag are byte-identical. ✓

3. Validation. statSync against resolve(project.dir, entryFile) at lines 277-285 with the bare catch {} → friendly errorBox + process.exit(1). Manual test in the PR body shows the actual error UX. The bare catch can't distinguish "file missing" from "permission denied" but for a local CLI both lead the user to the same investigation. ✓

4. Docker passthrough. Conditional --composition <entryFile> append at dockerRunArgs.ts:71. Two new tests at dockerRunArgs.test.ts:214-228 cover both branches (forwards when set, omits when not). ✓

5. Render plan output. nameLabel = entryFile ? project.name + "/" + entryFile : project.name makes it visible to the user which file is being rendered when -c is passed. Reads cleanly. ✓

6. Naming. --composition / -c is consistent with the existing HF CLI conventions (singular, kebab-case, lowercase short alias matching the first letter of the long form — -c for --composition, just like -o for --output). ✓

7. CI. All 42 checks green or in_progress (Render on windows-latest still running at review time). Lint, Typecheck, Build, Test, Test: runtime contract, CLI smoke, Tests on windows-latest, Smoke: global install — all pass.

Non-blocking observations

• Test gap on the CLI-side validation branch. The PR adds the existence-check logic in render.ts but only tests the Docker passthrough. A render.test.ts test for "render command surfaces errorBox and exits with code 1 when --composition points to a missing file" would round out the coverage. Manual test in the PR body covers the user-visible behavior so this isn't blocking, but worth a small follow-up commit.

• Path containment. --composition ../../../some/other/dir/file.html would pass both the CLI's existence check and the producer's existence check at server.ts:125. hyperframeLint.ts:55-57 has a directory-containment guard (if (!absoluteEntryPath.startsWith(absProjectDir))) but the render path in server.ts doesn't. For a local CLI run this is a UX point rather than a security boundary — the user has shell access to anything they could pass via -c. Worth mirroring the guard in server.ts for symmetry with the lint service, but that's a separate refactor that doesn't belong in this PR. Flagging only because it's a quiet inconsistency between two adjacent producer surfaces.

LGTM. Posted as COMMENT per the stamp policy.

— Rames Jusso

Verdict: comment — change is well-scoped and the docker plumbing is solid, but a few things should land before merge.

This is a clean exposure of an existing producer capability (entryFile) to the CLI surface. The diff is small, threads through both render paths, and adds the right kind of test for the docker arg wiring. Two real gaps below — neither is a hard blocker, but important #1 (path traversal) shouldn't ship without a fix or an explicit decision to defer.

Important

• packages/cli/src/commands/render.ts:273-287 — no path-traversal guard on --composition.
  resolve(project.dir, entryFile) followed by statSync will happily accept --composition ../../../etc/hosts (or anything outside project.dir). The exact same codebase already enforces this for entryFile in packages/producer/src/services/hyperframeLint.ts:55-58:

  const absoluteEntryPath = resolve(absProjectDir, entryFile);
  if (!absoluteEntryPath.startsWith(absProjectDir)) {
    return { error: `Entry file must stay inside project directory: ${entryFile}` };
  }

  Render should mirror that. The producer ultimately reads the file via join(projectDir, entryFile) in renderOrchestrator.ts:1974, so the CLI is the right place to fail fast with a clear error. The Docker mount is :ro which limits blast radius for the container path, but local renders run in the user's own FS context — escaping project.dir is real. Add the prefix check next to the statSync and re-use the lint message style.

• packages/cli/src/commands/render.ts — no test that entryFile is forwarded to createRenderJob.
  The existing test pattern is right there (render.test.ts:105-134 — "forwards parsed --variables payload to createRenderJob" / "omits variables ... when not provided"). The docker-side wiring is well-covered by the new dockerRunArgs.test.ts cases, but the local-render path has no equivalent assertion. A copy-paste of the variables test pattern with entryFile: "compositions/intro.html" would close this and match the file's existing convention. Without it, a future refactor of renderLocal could drop entryFile silently and CI would stay green.

• packages/cli/src/utils/dockerRunArgs.test.ts:150-179 — "regression tripwire" test not updated.
  The comment in that test explicitly says: "If a future option is added but only wired through to renderLocal, this test forces the author to update buildDockerRunArgs (and add a check here) too." The PR added a new option and added dedicated tests for it (good), but didn't extend the tripwire. That weakens the convention the file is trying to establish. Add entryFile: "compositions/intro.html" to the options and expect(args).toContain("--composition") to the assertions block.

Nits

• packages/cli/src/commands/render.ts:283 — error message points users at hyperframes compositions, but that command lists composition IDs (data-composition-id), not file paths.
  The source column does surface the file path for sub-compositions, which covers the main use case. Still, a user who has a top-level-only index.html will run hyperframes compositions, see an ID like intro, and try --composition intro — that'll fail. Consider either (a) updating the compositions output to always show a "render with: --composition " hint, or (b) softening the error to "Pass a path to a .html file relative to the project directory (e.g. compositions/intro.html)." Out of scope for this PR if you want, but worth a follow-up issue.

• packages/cli/src/commands/render.ts:294 — project.name + "/" + entryFile doesn't normalize.
  If someone runs --composition ./compositions/intro.html, the render-plan line prints myproj/./compositions/intro.html. Cosmetic only. entryFile.replace(/^\.\//, "") or relative() would be cleaner.

• packages/cli/src/commands/render.ts:66-71 — the description "Render a specific composition file instead of index.html" is good, but doesn't mention the engine's actual constraint: sub-compositions using <template> wrappers require the project's index.html to exist and reference the file via data-composition-src (see renderOrchestrator.ts:1984-2000). Worth a one-line "must be referenced from index.html via data-composition-src for sub-compositions" so users don't try to render an arbitrary HTML file and get a confusing error from the engine.

Praise

• Threading the option through both renderLocal and renderDocker in one PR (rather than the historical pattern of forgetting one path — see the --hdr regression that the dockerRunArgs file is now structured to prevent) is exactly right.
• The validation-then-pass-through approach (no engine changes, leverages existing producer config) is the minimum-blast-radius design. Good call.
• Both forward and omit cases tested for the docker arg builder — that's the right shape.

— Vai

Verdict: APPROVE — all three important items from my prior review addressed cleanly in 90399718. No new blockers.

Addressed since prior review

1. Path-traversal guard ✅ — packages/cli/src/commands/render.ts:280-286 (commit 90399718). Mirrors hyperframeLint.ts:55-58 exactly: resolves absProjectDir, checks entryPath.startsWith(absProjectDir), fails fast with errorBox + process.exit(1). Same pattern, same trade-offs as the existing producer-side guard — consistent.

2. Forward-test for entryFile → createRenderJob ✅ — packages/cli/src/commands/render.test.ts:136-151 (forward case) and :153-166 (omit case). Asserts producerState.createdJobs[0]?.entryFile === "compositions/intro.html" and the symmetric toBeUndefined() when the flag is absent. Covers the silent-drop failure mode I flagged.

3. Tripwire test extended ✅ — packages/cli/src/utils/dockerRunArgs.test.ts:164 adds entryFile: "compositions/intro.html" to options; :180-181 asserts args contains both --composition and compositions/intro.html. Future option additions will now hit the tripwire as intended.

Bonus nits also addressed

• render.ts:70-71 — description now calls out the <template> sub-composition / data-composition-src constraint. 👍
• render.ts:276 — .replace(/^\.\//, "") normalizes the ./ prefix so the render-plan label is clean.
• render.ts:291 — error message no longer points at hyperframes compositions; now suggests a .html path directly.

All three "skip-friendly" nits from the prior review picked up anyway — nice cleanup.

Still open

None.

New findings

None blocking. One observation, non-actionable: the startsWith(absProjectDir) check inherits the same sibling-prefix edge case as hyperframeLint.ts (e.g., a project at /foo/bar would technically not reject a resolved path at /foo/bar-evil/x). The producer-side guard has lived with this since it shipped, and consistency between the two call sites is more valuable than diverging here. If we ever want to harden it, do it in both places at once with path.relative + .. check.

— Vai

LGTM — re-review at 90399718. All concerns from my round-1 review and Vai's resolved cleanly in 90399718.

Prior-concern resolution

Vai's Importants:

• ✅ Path-traversal guard (#1) — render.ts:273-279 adds entryPath.startsWith(absProjectDir) check, mirrors hyperframeLint.ts:55-58 style and message
• ✅ entryFile forwarded to createRenderJob test (#2) — render.test.ts:136-166 adds two cases (forwards-when-set, omits-when-not), mirroring the existing variables test pattern at :105-134
• ✅ Tripwire test update (#3) — dockerRunArgs.test.ts:163 + :180-181 adds entryFile to options and --composition / compositions/intro.html to assertions, restoring the file's "every option must reach the container exactly once" invariant

Vai's Nits — all addressed:

• ✅ Description now mentions the data-composition-src constraint for sub-comps using <template> wrappers (render.ts:67-70)
• ✅ Path-normalization on ./ prefix (render.ts:274: .replace(/^\.\//, "")) — render-plan output and validation use the normalized form
• ✅ Error message replaced "Use hyperframes compositions to list available" → "Pass a path to a .html file relative to the project root (e.g. compositions/intro.html)." Avoids the IDs-vs-paths confusion Vai called out

My round-1 observations: both addressed (the test-gap was Vai's #2, the path-containment was Vai's #1 — my round-1 framing on both was looser, Vai's was specific. Coverage is identical.)

New finding from re-review (non-blocking, pre-existing pattern)

render.ts:276 — startsWith(absProjectDir) has a classic prefix pitfall.

const absProjectDir = resolve(project.dir);
const entryPath = resolve(absProjectDir, entryFile);
if (!entryPath.startsWith(absProjectDir)) { ... }

For a project at /home/user/coolproj/, passing --composition ../coolproj-evil/secret.html resolves to /home/user/coolproj-evil/secret.html, which startsWith("/home/user/coolproj") returns true (because coolproj-evil shares the prefix coolproj). The guard passes, and the producer happily renders a file from the sibling directory.

The fix is the standard one — require a path separator after the project dir, or use path.relative():

import { sep } from "node:path";
// either:
if (entryPath !== absProjectDir && !entryPath.startsWith(absProjectDir + sep)) { ... }
// or:
import { relative } from "node:path";
if (relative(absProjectDir, entryPath).startsWith("..")) { ... }

Why I'm calling this non-blocking:

1. Same pattern lives at packages/producer/src/services/hyperframeLint.ts:55-58 — this PR is mirroring an existing convention, and the bug is pre-existing on that surface too. A proper fix is "tighten both surfaces in one follow-up" rather than diverging this one.
2. For a local CLI, the user has shell access to anything they could pass via -c, so this is a UX/intent guard rather than a security boundary. A user could cp ../coolproj-evil/secret.html ./ and pass the local path with no guard at all.
3. Realistic exploit requires a sibling directory whose name happens to share the project dir's name as a prefix, which is unusual in practice.

That said — happy to file this as a separate "tighten path-containment guards across CLI + lint service" issue, or address it in this PR if you'd prefer the symmetry from the start. Either is fine.

Summary

Three rounds of review (mine round-1 + Vai's round-1 + this re-review), zero echo, all importants resolved, all nits resolved. Solid iteration loop. The new prefix-pitfall finding is a follow-up at most — the change is strictly better than master and the bug it inherits is already shared with the lint service.

Ship it.

— Re-review by Rames Jusso (pr-review)